Core: Add gpu rendering support #185

vaxerski · 2024-07-19T12:21:43Z

Adds support (enabled by default) for gpu rendering

Bugs:

dynamic wallpaper changes are black
there is a leak somewhere in preload (tho its also there on main I believe)

vaxerski · 2024-07-19T12:22:11Z

@fufexan nix fix plx

vaxerski · 2024-07-19T18:08:41Z

@gulafaran you did some memory leak analysis, I can't get valgrind or lsan to run on this, any ideas to track down the leak?

gulafaran · 2024-07-19T20:01:47Z

il poke around tomorrow and see if i can get it to find something useful

vaxerski · 2024-07-19T20:20:36Z

thanks bae

gulafaran · 2024-07-20T07:51:15Z

here is the temp changes i make to get asan to produce something.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index a398016..18271fd 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -121,7 +121,8 @@ set(CPACK_PROJECT_NAME ${PROJECT_NAME})
 set(CPACK_PROJECT_VERSION ${PROJECT_VERSION})
 include(CPack)
 
-target_link_libraries(hyprpaper PkgConfig::deps)
+target_link_libraries(hyprpaper -fsanitize=address,undefined PkgConfig::deps)
+target_compile_options(hyprpaper PUBLIC -fsanitize=address,undefined)
 
 target_link_libraries(
   hyprpaper
diff --git a/src/Hyprpaper.cpp b/src/Hyprpaper.cpp
index d0bddb6..ce7ac31 100644
--- a/src/Hyprpaper.cpp
+++ b/src/Hyprpaper.cpp
@@ -134,7 +134,8 @@ void CHyprpaper::init() {
 
     tick(true);
 
-    while (1) {
+    int count = 0;
+    while (count < 5) {
         int ret = poll(pollFDs, 2, 5000 /* 5 seconds, reasonable. Just in case we need to terminate and the signal fails */);
 
         if (ret < 0) {
@@ -174,6 +175,7 @@ void CHyprpaper::init() {
                 wl_display_flush(m_sDisplay);
             } while (ret2 > 0);
         }
+        count++;
     }
 
     unlockSingleInstance();

this PR has a new issue on destruction

/home/tom/dev/hyprpaper/src/render/Buffer.cpp:180:13: runtime error: member call on null pointer of type 'CEGL'
    #0 0x6b660e in CBuffer::destroyGpu() /home/tom/dev/hyprpaper/src/render/Buffer.cpp:180:13
    #1 0x6b6fd8 in CBuffer::~CBuffer() /home/tom/dev/hyprpaper/src/render/Buffer.cpp:199:9
    #2 0x70df21 in std::default_delete<CBuffer>::operator()(CBuffer*) const /usr/lib/gcc/x86_64-pc-linux-gnu/14/include/g++-v14/bits/unique_ptr.h:93:2
    #3 0x70dcd9 in Hyprutils::Memory::CSharedPointer_::impl<CBuffer>::_destroy() /usr/include/hyprutils/memory/./SharedPtr.hpp:69:21
    #4 0x70d4ed in Hyprutils::Memory::CSharedPointer_::impl<CBuffer>::destroy() /usr/include/hyprutils/memory/./SharedPtr.hpp:103:21
    #5 0x6183d5 in Hyprutils::Memory::CSharedPointer<CBuffer>::destroyImpl() /usr/include/hyprutils/memory/./SharedPtr.hpp:279:24
    #6 0x61822b in Hyprutils::Memory::CSharedPointer<CBuffer>::decrement() /usr/include/hyprutils/memory/./SharedPtr.hpp:265:21
    #7 0x617ee5 in Hyprutils::Memory::CSharedPointer<CBuffer>::~CSharedPointer() /usr/include/hyprutils/memory/./SharedPtr.hpp:177:17
    #8 0x617e89 in std::pair<SMonitor* const, Hyprutils::Memory::CSharedPointer<CBuffer>>::~pair() /usr/lib/gcc/x86_64-pc-linux-gnu/14/include/g++-v14/bits/stl_iterator.h:3001:12


SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/tom/dev/hyprpaper/src/render/Buffer.cpp:180:13 
/home/tom/dev/hyprpaper/src/render/Egl.cpp:160:12: runtime error: member call on null pointer of type 'CEGL *'
    #0 0x6c6358 in CEGL::destroyEglImage(void*) /home/tom/dev/hyprpaper/src/render/Egl.cpp
    #1 0x6b6674 in CBuffer::destroyGpu() /home/tom/dev/hyprpaper/src/render/Buffer.cpp:180:13
    #2 0x6b6fd8 in CBuffer::~CBuffer() /home/tom/dev/hyprpaper/src/render/Buffer.cpp:199:9
    #3 0x70df21 in std::default_delete<CBuffer>::operator()(CBuffer*) const /usr/lib/gcc/x86_64-pc-linux-gnu/14/include/g++-v14/bits/unique_ptr.h:93:2

im gonna have to hunt down a recent mesa shader cache fd leak before i dig any deeper but feel free to asan it you too :)

gulafaran · 2024-07-20T08:03:16Z

there is also

hyprpaper/src/render/LayerSurface.cpp

Line 38 in d82aec0

    
           pLayerSurface->sendSetAnchor((zwlrLayerSurfaceV1Anchor)(ZWLR_LAYER_SURFACE_V1_ANCHOR_TOP | ZWLR_LAYER_SURFACE_V1_ANCHOR_RIGHT | ZWLR_LAYER_SURFACE_V1_ANCHOR_BOTTOM |

, casting the enum to the value 15.

enum zwlrLayerSurfaceV1Anchor : uint32_t {
    ZWLR_LAYER_SURFACE_V1_ANCHOR_TOP = 1,
    ZWLR_LAYER_SURFACE_V1_ANCHOR_BOTTOM = 2,
    ZWLR_LAYER_SURFACE_V1_ANCHOR_LEFT = 4,
    ZWLR_LAYER_SURFACE_V1_ANCHOR_RIGHT = 8,
};

vaxerski · 2024-07-20T10:53:28Z

enum casting out of range is a classic in wayland I don't think we gon' escape that one.

Is it still UB even though we define it as uint32_t?

gulafaran · 2024-07-20T10:59:51Z

enum casting out of range is a classic in wayland I don't think we gon' escape that one.

Is it still UB even though we define it as uint32_t?

well ZWLR_LAYER_SURFACE_V1_ANCHOR_TOP | ZWLR_LAYER_SURFACE_V1_ANCHOR_RIGHT | ZWLR_LAYER_SURFACE_V1_ANCHOR_BOTTOM | ZWLR_LAYER_SURFACE_V1_ANCHOR_LEFT as in 1 + 8 + 2 + 4 = 15, cast uint32_t value 15 to (zwlrLayerSurfaceV1Anchor) enum but it has no member with value 15, i would assume its all sorts of UB. i wonder how kwin deals with things like this since its also c++ based hm. does it just send and deal with uint32 directly and parse the values to either single members depending on value or multiple if no member has the value? are the protocols really sending enums or uint32_t's? i suspect uint32_t without reading on it which mean its an issue on "our" side in parsing things and not in the protocol

gulafaran · 2024-07-20T11:14:51Z

for example in wlroots,

struct wlr_layer_surface_v1_state {
	uint32_t committed; // enum wlr_layer_surface_v1_state_field

	uint32_t anchor;

static void layer_surface_exclusive_zone(
		struct wlr_layer_surface_v1_state *state,
		struct wlr_box *usable_area) {
	switch (state->anchor) {

static void layer_surface_handle_set_anchor(struct wl_client *client,
		struct wl_resource *resource, uint32_t anchor) {
	const uint32_t max_anchor =
		ZWLR_LAYER_SURFACE_V1_ANCHOR_TOP |
		ZWLR_LAYER_SURFACE_V1_ANCHOR_BOTTOM |
		ZWLR_LAYER_SURFACE_V1_ANCHOR_LEFT |
		ZWLR_LAYER_SURFACE_V1_ANCHOR_RIGHT;

    if (anchor > max_anchor) {

it seems to be dealing with uint32_t all over and just using the enum values for references to what is what. it never casts it out of range that is

gulafaran · 2024-07-20T11:27:30Z

in short, im not fully read up how things are built up to a working source but i suspect hyprwayland-scanner builds the protocol things from the .xml files. perhaps it should just use the datatype of the protocol as parameters to the functions.

<request name="set_anchor">
      <description summary="configures the anchor point of the surface">
        Requests that the compositor anchor the surface to the specified edges
        and corners. If two orthoginal edges are specified (e.g. 'top' and
        'left'), then the anchor point will be the intersection of the edges
        (e.g. the top left corner of the output); otherwise the anchor point
        will be centered on that edge, or in the center if none is specified.

        Anchor is double-buffered, see wl_surface.commit.
      </description>
      <arg name="anchor" type="uint" enum="anchor"/>
    </request>

and on the sending and recieving side you still use uint32_t and just check its values if they are within range of the enum or exact values of the enum. and the enum decides what values does what or not. instead of trying to cast the enum to a value and send that as a parameter and as a consequence UB and out of range casting when all it does is technically send uint32_t.

just an idea from a random newb. :D , god i suck at trying to explain what im thinking too.

vaxerski · 2024-07-20T15:28:40Z

hw-s puts the enums there so that you know what it is. In actuality, it's just used as an uint32_t.

gulafaran · 2024-07-20T17:15:30Z

hw-s puts the enums there so that you know what it is. In actuality, it's just used as an uint32_t.

but what is creating sendSetAnchor and its parameters?

gulafaran · 2024-07-20T17:21:39Z

hw-s puts the enums there so that you know what it is. In actuality, it's just used as an uint32_t.

but what is creating sendSetAnchor and its parameters?

because thats what we should be changing to uint32_t instead.

like

wlr-layer-shell-unstable.hpp

-void sendSetAnchor(zwlrLayerSurfaceV1Anchor);
+void sendSetAnchor(uint32_t);

wlr-layer-shell-unstable.cpp

-void CCZwlrLayerSurfaceV1::sendSetAnchor(zwlrLayerSurfaceV1Anchor anchor) {
+void CCZwlrLayerSurfaceV1::sendSetAnchor(uint32_t anchor) {
    if (!pResource)
        return;

    auto proxy = wl_proxy_marshal_flags((wl_proxy*)pResource, 1, nullptr, wl_proxy_get_version((wl_proxy*)pResource), 0, anchor);
    proxy;
}

wl_proxy_marshal_flags already takes an uint32_t as its flags.

meaning in layersurface.cpp it can use

pLayerSurface->sendSetAnchor((ZWLR_LAYER_SURFACE_V1_ANCHOR_TOP | ZWLR_LAYER_SURFACE_V1_ANCHOR_RIGHT | ZWLR_LAYER_SURFACE_V1_ANCHOR_BOTTOM | ZWLR_LAYER_SURFACE_V1_ANCHOR_LEFT));

and then there is no casting out of range. same deal with all the out of casting enums in hyprland and the other tools.

vaxerski · 2024-07-20T18:57:53Z

but then you could mistakenly take the wrong enum

gulafaran · 2024-07-20T19:13:04Z

but then you could mistakenly take the wrong enum

well imo rather that then UB, one of the key issues with UB is it can show itself just as in recent conversations in hyprwm/Hyprland#6608 "working" in debug builds but as fast as higher optimisation levels are applied the compiler just strips out things that should be there. and crashes on release builds. or even cause really wonky memory issues. unless you got any other ideas i would go with the route of maybe wrong values instead of relying on chance the compiler gets it somewhat right ("it usually doesnt") , in this case who knows what it does its undefined after all but what if its doing anything like this famous horror example https://godbolt.org/z/3Pqojanqo eraseall is never called and yet it is.

implentation bugs versus language/compiler bugs sort of. one is solveable one isnt

vaxerski · 2024-07-20T19:18:39Z

lmao the linked one is indeed a disaster

I don't think the compiler is allowed to optimize here as we added : uint32_t, but I could just change the enum types to uint32_t I guess.

vaxerski · 2024-07-20T19:21:37Z

pushed to hw-s

vaxerski · 2024-07-20T19:21:53Z

hyprwm/hyprwayland-scanner@ce8dae8

gulafaran · 2024-07-20T19:32:54Z

hyprwm/hyprwayland-scanner@ce8dae8

cool :) now just a few minor changes in the hyprwm land then. heres hyperpaper., il see if i got the time to dig around the other tools tomorrow for out of range casts. trying to repaint the house and they are showing good weather tomorrow so a bit busy with that too heh

diff --git a/src/Hyprpaper.cpp b/src/Hyprpaper.cpp
index d0bddb6..c31ca82 100644
--- a/src/Hyprpaper.cpp
+++ b/src/Hyprpaper.cpp
@@ -258,7 +258,7 @@ void CHyprpaper::recheckAllMonitors() {
 void CHyprpaper::createSeat(SP<CCWlSeat> pSeat) {
     m_pSeat = pSeat;
 
-    pSeat->setCapabilities([this](CCWlSeat* r, wl_seat_capability caps) {
+    pSeat->setCapabilities([this](CCWlSeat* r, uint32_t caps) {
         if (caps & WL_SEAT_CAPABILITY_POINTER) {
             m_pSeatPointer = makeShared<CCWlPointer>(m_pSeat->sendGetPointer());
             if (!m_pCursorShape)
diff --git a/src/render/LayerSurface.cpp b/src/render/LayerSurface.cpp
index e4c969d..36f288a 100644
--- a/src/render/LayerSurface.cpp
+++ b/src/render/LayerSurface.cpp
@@ -35,7 +35,7 @@ CLayerSurface::CLayerSurface(SMonitor* pMonitor) {
     }
 
     pLayerSurface->sendSetSize(0, 0);
-    pLayerSurface->sendSetAnchor((zwlrLayerSurfaceV1Anchor)(ZWLR_LAYER_SURFACE_V1_ANCHOR_TOP | ZWLR_LAYER_SURFACE_V1_ANCHOR_RIGHT | ZWLR_LAYER_SURFACE_V1_ANCHOR_BOTTOM |
+    pLayerSurface->sendSetAnchor((ZWLR_LAYER_SURFACE_V1_ANCHOR_TOP | ZWLR_LAYER_SURFACE_V1_ANCHOR_RIGHT | ZWLR_LAYER_SURFACE_V1_ANCHOR_BOTTOM |
                                                             ZWLR_LAYER_SURFACE_V1_ANCHOR_LEFT));
     pLayerSurface->sendSetExclusiveZone(-1);

vaxerski · 2024-07-20T19:45:27Z

no hurries, everything should still compile.

If you could just make MRs whenever you have a moment

vaxerski · 2024-07-20T21:21:50Z

I've dropped the commit as its a breaking change... we gotta figure out a different way.

One thing of note: doesn't wayland itself do this all the time?

vaxerski · 2024-07-20T21:23:13Z

hm, no, it uses uint32_t... I don't know how to make this non-breaking, though. Any ideas?

vaxerski · 2024-07-20T21:25:47Z

Is this actually UB? The values are fixed and specified as uint32_t.

vaxerski · 2024-07-20T21:27:36Z

Seems like we are not doing UB.

gulafaran · 2024-07-20T22:14:30Z

Seems like we are not doing UB.

Had to bring out the big ctrl + f in the c++ std

(...) If the enumeration type has a fixed underlying type, the value is first converted to that type by integral conversion, if necessary, and then to the enumeration type. If the enumeration type does not have a fixed underlying type, the value is unchanged if the original value is within the range of the enumeration values (9.7.1), and otherwise, the behavior is undefined.

Indeed seems c++ allow this as long as the type is fixed
with a few notes as mentioned in that picture, one learns everyday. Atleast looks like so

vaxerski · 2024-07-20T22:25:09Z

:D yours truly

vaxerski · 2024-07-23T21:12:03Z

I've verified the leak is not there on main

also @gulafaran do you have an idea why gbm_device_create ups the VmRSS like 120MB???

gulafaran · 2024-07-24T17:51:50Z

I've verified the leak is not there on main

also @gulafaran do you have an idea why gbm_device_create ups the VmRSS like 120MB???

tried fiddling with asan/valgrind im not sure why something sneaky is going on asan runs it but valgrind segfaults

 6 ==8165== Syscall param ioctl(DRM_VERSION).version_major points to unaddressable byte(s)
  7 ==8165==    at 0x5547766: ioctl (in /usr/lib64/libc.so.6)
  8 ==8165==    by 0x4929250: drmGetVersion (in /usr/lib64/libdrm.so.2.4.0)
  9 ==8165==    by 0x4902192: ??? (in /usr/lib64/libgbm.so.1.0.0)
 10 ==8165==    by 0x4901398: ??? (in /usr/lib64/libgbm.so.1.0.0)
 11 ==8165==    by 0x48FF3A7: ??? (in /usr/lib64/libgbm.so.1.0.0)
 12 ==8165==    by 0x48FF52C: gbm_create_device (in /usr/lib64/libgbm.so.1.0.0)
 13 ==8165==    by 0x450BD8: CRenderer::CRenderer(bool) (src/render/Renderer.cpp:149)
 14 ==8165==    by 0x4113B2: std::__detail::_MakeUniq<CRenderer>::__single_object std::make_unique<CRenderer, bool>(bool&&) (unique_ptr.h:107    6)
 15 ==8165==    by 0x40AA3B: CHyprpaper::init() (src/Hyprpaper.cpp:91)
 16 ==8165==    by 0x43FFD8: main (src/main.cpp:36)
 17 ==8165==  Address 0x1008f42bb0 is 0 bytes inside a block of size 80 in arena "core"

either im doing something wrong or something very wonky is going on heh

vaxerski · 2024-07-24T17:57:29Z

thats not my code :)

gulafaran · 2024-07-24T18:00:21Z

thats not my code :)

yeah i might be hitting some mesa-git funzies but asan doesnt report anything nasty and the gl/shader code is beyond my knowledge are they somehow doing something? like missing glDeleteProgram or glDetachShader

vaxerski · 2024-07-24T18:14:53Z

would be odd cuz we leaking ram not vram

basic gpu stuff

c788a59

vaxerski and others added 2 commits July 19, 2024 16:27

fixez

518213b

Nix: add mesa dep

d82aec0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Add gpu rendering support #185

Core: Add gpu rendering support #185

vaxerski commented Jul 19, 2024 •

edited

Loading

vaxerski commented Jul 19, 2024

vaxerski commented Jul 19, 2024

gulafaran commented Jul 19, 2024

vaxerski commented Jul 19, 2024

gulafaran commented Jul 20, 2024

gulafaran commented Jul 20, 2024

vaxerski commented Jul 20, 2024

gulafaran commented Jul 20, 2024

gulafaran commented Jul 20, 2024

gulafaran commented Jul 20, 2024

vaxerski commented Jul 20, 2024

gulafaran commented Jul 20, 2024

gulafaran commented Jul 20, 2024

vaxerski commented Jul 20, 2024

gulafaran commented Jul 20, 2024

vaxerski commented Jul 20, 2024

vaxerski commented Jul 20, 2024

vaxerski commented Jul 20, 2024

gulafaran commented Jul 20, 2024

vaxerski commented Jul 20, 2024

vaxerski commented Jul 20, 2024

vaxerski commented Jul 20, 2024 •

edited

Loading

vaxerski commented Jul 20, 2024

vaxerski commented Jul 20, 2024

gulafaran commented Jul 20, 2024

vaxerski commented Jul 20, 2024

vaxerski commented Jul 23, 2024

gulafaran commented Jul 24, 2024

vaxerski commented Jul 24, 2024

gulafaran commented Jul 24, 2024

vaxerski commented Jul 24, 2024

Core: Add gpu rendering support #185

Are you sure you want to change the base?

Core: Add gpu rendering support #185

Conversation

vaxerski commented Jul 19, 2024 • edited Loading

vaxerski commented Jul 19, 2024

vaxerski commented Jul 19, 2024

gulafaran commented Jul 19, 2024

vaxerski commented Jul 19, 2024

gulafaran commented Jul 20, 2024

gulafaran commented Jul 20, 2024

vaxerski commented Jul 20, 2024

gulafaran commented Jul 20, 2024

gulafaran commented Jul 20, 2024

gulafaran commented Jul 20, 2024

vaxerski commented Jul 20, 2024

gulafaran commented Jul 20, 2024

gulafaran commented Jul 20, 2024

vaxerski commented Jul 20, 2024

gulafaran commented Jul 20, 2024

vaxerski commented Jul 20, 2024

vaxerski commented Jul 20, 2024

vaxerski commented Jul 20, 2024

gulafaran commented Jul 20, 2024

vaxerski commented Jul 20, 2024

vaxerski commented Jul 20, 2024

vaxerski commented Jul 20, 2024 • edited Loading

vaxerski commented Jul 20, 2024

vaxerski commented Jul 20, 2024

gulafaran commented Jul 20, 2024

vaxerski commented Jul 20, 2024

vaxerski commented Jul 23, 2024

gulafaran commented Jul 24, 2024

vaxerski commented Jul 24, 2024

gulafaran commented Jul 24, 2024

vaxerski commented Jul 24, 2024

vaxerski commented Jul 19, 2024 •

edited

Loading

vaxerski commented Jul 20, 2024 •

edited

Loading